Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update flow framework additional fields in previous_node_inputs #8233

Merged

Conversation

yuye-aws
Copy link
Member

@yuye-aws yuye-aws commented Sep 12, 2024

Description

Flow framework supports parse connector_id and agent_id from previous steps when create tools.

Issues Resolved

Closes #8232

Version

List the OpenSearch version to which this PR applies, e.g. 2.14, 2.12--2.14, or all.

Since 2.17.

Frontend features

If you're submitting documentation for an OpenSearch Dashboards feature, add a video that shows how a user will interact with the UI step by step. A voiceover is optional.

Checklist

  • By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and subject to the Developers Certificate of Origin.
    For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link

Thank you for submitting your PR. The PR states are In progress (or Draft) -> Tech review -> Doc review -> Editorial review -> Merged.

Before you submit your PR for doc review, make sure the content is technically accurate. If you need help finding a tech reviewer, tag a maintainer.

When you're ready for doc review, tag the assignee of this PR. The doc reviewer may push edits to the PR directly or leave comments and editorial suggestions for you to address (let us know in a comment if you have a preference). The doc reviewer will arrange for an editorial review.

@yuye-aws
Copy link
Member Author

@dbwiddis Can you also take a look?

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to have all of these descriptions very similar in what they say. I don't think we need to explicitly list when they are already included as required inputs shown on the workflow step API, so the current model ID phrase as "input for several steps" is appropriate.

What needs to be called out here is unusual things:

  1. the llm.mode_id vs. model_id difference (already there)
  2. the inclusion of all 3 of these in the parameters key for tools (not yet here)

_automating-configurations/workflow-steps.md Outdated Show resolved Hide resolved
_automating-configurations/workflow-steps.md Outdated Show resolved Hide resolved
|-----------------|--- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `model_id` |String | The `model_id` is used as an input for several steps. As a special case for the Register Agent step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
| `agent_id` |String | The `agend_id` is used as an input for `delete_agent` and `create_tool` step. |
| `connector_id` |String | The `connector_id` is used as an input for `delete_connector`, `register_remote_model` and `create_tool` steps. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a key difference here. For register remote model and delete connector, the connector ID is explicitly listed as a required input which would show up with the workflow steps API, and I don't think that needs explicit documentation (other than a similar line as we have for model ID). For the create tool, it's included as part of the parameters, and this I think is the important point that needs to be called out here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I need to explicitly show the create_tool type for connector_id is only used for connector tool.

@dbwiddis dbwiddis added the 3 - Tech review PR: Tech review in progress label Sep 12, 2024
|`model_id` |String |The `model_id` is used as an input for several steps. As a special case for the Register Agent step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
| Field |Data type | Description |
|-----------------|--- |--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `model_id` |String | The `model_id` is used as an input for `delete_model`, `deploy_model`, `undeploy_model`, `register_agent` and `create_tool` step. As a special case for the `register_agent` step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still missing the fact that for create_tool, it's part of the parameters input.

That's really the essential information you're adding with this documentation update but it's not obvious to me and I've looked at the code.

I really don't think you need to list every step that it's a required input for; those are included in the API already. The previous line "several steps" is fine, perhaps adding a reference to using that workflow step API.

Please focus the new content on what is not clearly identified by the existing API.

Copy link
Member Author

@yuye-aws yuye-aws Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Updated.

|`model_id` |String |The `model_id` is used as an input for several steps. As a special case for the Register Agent step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
| Field |Data type | Description |
|-----------------|--- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `model_id` |String | The `model_id` is used as an input for several steps. As a special case for the `register_agent` step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
Copy link
Member

@dbwiddis dbwiddis Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| `model_id` |String | The `model_id` is used as an input for several steps. As a special case for the `register_agent` step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
| `model_id` |String | The `model_id` is used as an input for several steps. As a special case for the `register_agent` step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. The `model_id` will also be included in the `parameters` input of the `create_tool` step. |

I would prefer this (repeat for the other two) rather than a separate "note".

Copy link
Member Author

@yuye-aws yuye-aws Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack. Updated.

yuye-aws and others added 2 commits September 12, 2024 14:14
Co-authored-by: Daniel Widdis <[email protected]>
Signed-off-by: yuye-aws <[email protected]>
Signed-off-by: yuye-aws <[email protected]>
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for taking the feedback.

@dbwiddis dbwiddis added 4 - Doc review PR: Doc review in progress v2.17.0 and removed 3 - Tech review PR: Tech review in progress labels Sep 12, 2024
@kolchfa-aws kolchfa-aws added the release-notes PR: Include this PR in the automated release notes label Sep 12, 2024
Copy link
Collaborator

@kolchfa-aws kolchfa-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@kolchfa-aws kolchfa-aws left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @yuye-aws!

Copy link
Collaborator

@natebower natebower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuye-aws @kolchfa-aws Please see my comments and changes and let me know if you have any questions. Thanks!

_automating-configurations/workflow-steps.md Outdated Show resolved Hide resolved
|--- |--- |--- |
|`model_id` |String |The `model_id` is used as an input for several steps. As a special case for the Register Agent step type, if an `llm.model_id` field is not present in the `user_inputs` and not present in `previous_node_inputs`, the `model_id` field from the previous node may be used as a backup for the model ID. |
| Field |Data type | Description |
|-----------------|--- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below: "As a special case" reads a bit awkwardly/ambiguously. Can it be removed/rephrased?

Copy link
Member Author

@yuye-aws yuye-aws Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with a note below.

_automating-configurations/workflow-steps.md Outdated Show resolved Hide resolved
_automating-configurations/workflow-steps.md Outdated Show resolved Hide resolved
yuye-aws and others added 3 commits September 13, 2024 22:08
@kolchfa-aws kolchfa-aws merged commit 39d56c4 into opensearch-project:main Sep 13, 2024
6 checks passed
noahstaveley pushed a commit to noahstaveley/documentation-website that referenced this pull request Sep 23, 2024
…search-project#8233)

* update: flow framework previous node inputs

Signed-off-by: yuye-aws <[email protected]>

* update typo

Signed-off-by: yuye-aws <[email protected]>

* update model id step input types

Signed-off-by: yuye-aws <[email protected]>

* update create_tool note

Signed-off-by: yuye-aws <[email protected]>

* reduce redundant step types

Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: Daniel Widdis <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* optimize doc

Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: kolchfa-aws <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: kolchfa-aws <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: kolchfa-aws <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

* Update _automating-configurations/workflow-steps.md

Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: yuye-aws <[email protected]>

---------

Signed-off-by: yuye-aws <[email protected]>
Co-authored-by: Daniel Widdis <[email protected]>
Co-authored-by: kolchfa-aws <[email protected]>
Co-authored-by: Nathan Bower <[email protected]>
Signed-off-by: Noah Staveley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Doc review PR: Doc review in progress release-notes PR: Include this PR in the automated release notes v2.17.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Update flow framework additional fields in previous_node_inputs
4 participants